Skip to content

ARM32 assembly: Get CPU Id flags to choose assembly.#10957

Open
SparkiDev wants to merge 1 commit into
wolfSSL:masterfrom
SparkiDev:arm32_cpuid_flags
Open

ARM32 assembly: Get CPU Id flags to choose assembly.#10957
SparkiDev wants to merge 1 commit into
wolfSSL:masterfrom
SparkiDev:arm32_cpuid_flags

Conversation

@SparkiDev

Copy link
Copy Markdown
Contributor

Description

Added flag determination for Linux/Android/BSD/Windows/privilege-mode.
Made AES and SHA-256 runtime dispatch.
Updated the ARM32 assembly to support having multiple implementations compiled in.

Testing

Tested ARM32 builds.

@SparkiDev SparkiDev self-assigned this Jul 21, 2026

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #10957

Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment thread wolfcrypt/src/aes.c
Comment thread wolfcrypt/src/aes.c
Comment thread wolfcrypt/src/aes.c
Comment thread wolfcrypt/src/aes.c
@SparkiDev
SparkiDev force-pushed the arm32_cpuid_flags branch from 8f05e85 to 3883e89 Compare July 21, 2026 01:38
@SparkiDev

Copy link
Copy Markdown
Contributor Author

Jenkins: retest this please

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #10957

Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src

No new issues found in the changed files. ✅

Added flag determination for Linux/Android/BSD/Windows/privilege-mode.
Made AES and SHA-256 runtime dispatch.
Updated the ARM32 assembly to support having multiple implementations compiled
in.
@SparkiDev
SparkiDev force-pushed the arm32_cpuid_flags branch from 3883e89 to 334bde6 Compare July 22, 2026 08:28
@SparkiDev

Copy link
Copy Markdown
Contributor Author

Jenkins: retest this please

@SparkiDev SparkiDev assigned wolfSSL-Bot and unassigned SparkiDev Jul 22, 2026
@SparkiDev
SparkiDev requested a review from wolfSSL-Bot July 22, 2026 12:53

@Frauschi Frauschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐺 Skoll Code Review

Overall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped

Posted findings

  • [Medium] OpenBSD grouped with FreeBSD but calls FreeBSD-only elf_aux_info()wolfcrypt/src/cpuid.c:719-767
  • [Low] Android CPUID branch is unreachable (falls into the linux branch first)wolfcrypt/src/cpuid.c:639-718
  • [Low] WOLFSSL_ARM32_AES_DISPATCH does not exclude Thumb-2 (unlike SHA256_ARM32_DISPATCH)wolfcrypt/src/aes.c:1094-1097

Review generated by Skoll via Claude/Codex

Comment thread wolfcrypt/src/cpuid.c
(&cpuid_flags, &old_cpuid_flags, new_cpuid_flags);
}
}
#elif defined(__FreeBSD__) || defined(__OpenBSD__)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] OpenBSD grouped with FreeBSD but calls FreeBSD-only elf_aux_info()
💡 SUGGEST

The new AArch32 feature-detection block adds a branch #elif defined(__FreeBSD__) || defined(__OpenBSD__) that #include <sys/auxv.h> and calls elf_aux_info(AT_HWCAP, ...) / elf_aux_info(AT_HWCAP2, ...). elf_aux_info() is a FreeBSD/NetBSD interface; OpenBSD does not provide it (nor the same <sys/auxv.h>/elf_aux_info API for reading HWCAP). On a 32-bit ARM (armv7) OpenBSD build with WOLFSSL_ARMASM, this branch is selected and the translation unit fails to compile/link (implicit declaration of elf_aux_info, or missing header), breaking cpuid.c for that supported platform. FreeBSD is unaffected. Because __OpenBSD__ is grouped here rather than falling through to the safe #else compiled-features fallback, the run-time-dispatch feature turns a previously buildable configuration into a build break.

Recommendation: Restrict this branch to platforms that actually provide elf_aux_info() (e.g. #elif defined(__FreeBSD__)), and give OpenBSD its own branch using its supported mechanism (e.g. sysctl CTL_MACHDEP) or let OpenBSD fall through to the #else compiled-features fallback.

Comment thread wolfcrypt/src/cpuid.c
(&cpuid_flags, &old_cpuid_flags, new_cpuid_flags);
}
}
#elif defined(__linux__)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] Android CPUID branch is unreachable (falls into the linux branch first)
💡 SUGGEST

In the new HAVE_CPUID_ARM32 block the platform selection places #elif defined(__linux__) (getauxval, line 639) before #elif defined(__ANDROID__) || defined(ANDROID) (cpu-features.h / android_getCpuFeatures, line 689). Android NDK/bionic toolchains define __linux__ in addition to __ANDROID__, so the __linux__ branch always matches first and the dedicated Android branch is dead code that is never compiled. On modern Android the getauxval/AT_HWCAP2 path works (API 18+/21+), so run-time detection still functions, but the intended NDK cpu-features fallback (written for pre-API-18 bionic that lacked getauxval) can never be reached. Note this exactly mirrors the pre-existing AArch64 code (cpuid.c:267 vs 329), so it may be an accepted convention rather than an oversight. Severity assessed as Low by both modes; confidence bumped to High on cross-mode agreement.

Recommendation: Order the #elif defined(__ANDROID__) || defined(ANDROID) branch before the generic #elif defined(__linux__) branch (in both the ARM32 and, for consistency, the AArch64 blocks) so the Android-specific detection is actually selectable. If getauxval is preferred everywhere, consider dropping the dead Android branch instead.

Comment thread wolfcrypt/src/aes.c
* base fallback can be dropped with WOLFSSL_ARMASM_NO_BASE_IMPL (crypto
* always present), and WOLFSSL_ARMASM_NO_HW_CRYPTO keeps only the base, both
* of which revert to direct calls with no run-time check. */
#if !defined(__aarch64__) && !defined(WOLFSSL_ARMASM_NO_HW_CRYPTO) && \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] WOLFSSL_ARM32_AES_DISPATCH does not exclude Thumb-2 (unlike SHA256_ARM32_DISPATCH)
🔧 NIT

The SHA-256 dispatch guard explicitly excludes Thumb-2 (sha256.c:1350 uses !defined(WOLFSSL_ARMASM_THUMB2)), but the analogous AES guard WOLFSSL_ARM32_AES_DISPATCH (#if !defined(__aarch64__) && !defined(WOLFSSL_ARMASM_NO_HW_CRYPTO) && !defined(WOLFSSL_ARMASM_NO_BASE_IMPL)) does not. The Thumb-2 AES assembly (thumb2-aes-asm.S) provides only the base implementation and none of the crypto-extension AES_*_AARCH32 symbols, so a Thumb-2 build that reaches this dispatch path would fail to link against AES_encrypt_AARCH32. This is safe in autotools builds because configure always pairs WOLFSSL_ARMASM_THUMB2 with WOLFSSL_ARMASM_NO_HW_CRYPTO (configure.ac:4139-4140), which suppresses the dispatch macro. It could bite a hand-rolled user_settings.h that sets WOLFSSL_ARMASM_THUMB2 + crypto without NO_HW_CRYPTO (already broken pre-diff, so not a regression — just missing defense-in-depth/consistency with the SHA-256 guard).

Recommendation: Add !defined(WOLFSSL_ARMASM_THUMB2) to the WOLFSSL_ARM32_AES_DISPATCH definition to match the SHA-256 dispatch guard and make the Thumb-2 restriction explicit at the code level rather than relying solely on configure.

@Frauschi Frauschi assigned SparkiDev and unassigned wolfSSL-Bot Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants